Conversation
jprestwo
left a comment
jprestwo
left a comment
There was a problem hiding this comment.
Mostly just questions about UV, no obvious problems I can see.
| INPUT_REQUIREMENTS requirements.in | ||
| USE_UV TRUE | ||
| UV_CACHE /tmp/testing/uv/cache | ||
| ) |
There was a problem hiding this comment.
I know its just a draft, but I assume this will be the default? It would just be nice to enable this transparently without having to change any packages, if that is at all possible.
Also, as far as the cache location, what is the consequence if different packages set different cache locations? They just won't be able to share the venv I assume?
There was a problem hiding this comment.
We wouldn't want this in /tmp because it would not survive a reboot. It should probably land in /var/lib/uv or whatever the current standard for persistent data is on Linux.
| set(use_uv "--use-uv") | ||
|
|
||
| if( ARG_ISOLATE_REQUIREMENTS ) | ||
| message( FATAL_ERROR "You can use ISOLATE_REQUIREMENTS in conjunction with USE_UV" ) |
There was a problem hiding this comment.
Did you mean You can't use ISOLATE_REQUIREMENTS...
There was a problem hiding this comment.
Depends on what that means. Do you not want to use uv at all, do you want your own cache (why?)
I'll have to look at what ISOLATE_REQUIREMENTS means a bit more, for some reason I thought it had to do with venv chaining and not necessarily using system-packages
There was a problem hiding this comment.
ISOLATE_REQUIREMENTS does indeed cause a package not to inherit its dependencies requirements.txt. Interestingly enough we don't seem to use it anywhere anymore, and I can't remember why we added it. However, does beg the questions why it's not supported with uv
| USE_UV TRUE | ||
| UV_CACHE /tmp/testing/uv/cache | ||
| EXTRA_PIP_ARGS .... | ||
| ) |
There was a problem hiding this comment.
My only concern here is if we have packages that use the extra pip args, and if they do for what reason and would this be supported with UV.
There was a problem hiding this comment.
The issue with EXTRA_PIP_ARGS is they get passed in as string to uv some pip args aren't supported.
I thought it cleaner to say: use these args if using uv use these args if using pip.
Admittedly some of the args will overlap.
|
|
||
|
|
||
| def run_command(cmd, *args, **kwargs): | ||
| def run_command(cmd: typing.List[str], *args, **kwargs) -> subprocess.CompletedProcess: |
There was a problem hiding this comment.
Not really your problem as this already existed, but we really need to move this into locus_common or somewhere. I keep seeing the same block of code copied into various packages 🤦
There was a problem hiding this comment.
catkin_virtualenv is a public package
| """Initialize a new uv virtualenv using the specified python version and extra arguments.""" | ||
|
|
||
| """ | ||
| This differs vrom the VirtualEnv class mainly in that the VirtualEnv class has to determine, |
|
|
||
|
|
||
| def run_command(cmd, *args, **kwargs): | ||
| def run_command(cmd: typing.List[str], *args, **kwargs) -> subprocess.CompletedProcess: |
There was a problem hiding this comment.
Do we need the log file as part of the PR, or is this meant to act of what it should look like?
|
Will review in more detail, but my biggest upfront concern is that to leverage this, you have to figure out a way to distribute the uv cache as part of the bundle deb. Replacing pip with another tool is frankly, scary. My philosophy with python packaging was historically 'avoid the newest sexy pip replacement because it will inevitably go out of style and be replaced with something else (pipenv, poetry, uv, what-have-you). Eventually the best stuff does get rolled into setuptools/pip. My parallel effort to remove redundant files was https://github.com/locusrobotics/tailor-distro/pull/88/files, which I think conceptually (and in practice) is a lot simpler as a post-processing step. There's an extra hoop I didn't jump through to make deb packaging honor symlinks, but if you want to reap the benefits of this in the image builds without much added work, you can add:
|
|
|
||
| catkin_run_tests_target("venv_check" "${PROJECT_NAME}-requirements" "venv_check-${PROJECT_NAME}-requirements.xml" | ||
| COMMAND "${CATKIN_ENV} rosrun catkin_virtualenv venv_check ${venv_dir} --requirements ${package_requirements} \ | ||
| --use-uv \ |
There was a problem hiding this comment.
one easy pattern to reduce duplication here:
if(DEFINED ARG_USE_UV)
set(extra_args "${extra_args} --use-uv")
endif()
catkin_run_tests_target(... COMMAND ... ${extra_args}
| self.path: pathlib.Path = path | ||
|
|
||
| if self.path in protected_paths: | ||
| raise RuntimeError(f"Cannot install into {self.path}") |
There was a problem hiding this comment.
I imagine these paths are protected by the nature of filesystem permissions? Do we really need to check this list at the application level?
4 participants
No description provided.